Plan: Add the whitaker_support_macros proc-macro crate (1.3.1) - #352
Plan: Add the whitaker_support_macros proc-macro crate (1.3.1)#352leynos wants to merge 21 commits into
Conversation
Promote the root package's clippy policy to `[workspace.lints]` so every member crate — including the rustc proxy shims and support crates — is held to the same standard, and extend it: - add `disallowed_methods` and `missing_assert_message` to the clippy set; - forbid `unsafe_code` workspace-wide; - add `[lints] workspace = true` to every member crate, removing the now-redundant local `expect_used`/`unwrap_used` and `unexpected_cfgs` tables; - introduce `.rustfmt.toml` with the estate's unstable nightly formatting options; - run workspace doctests (`cargo test --workspace --doc --all-features`) from the `test` target with warnings denied; - request the `rust-analyzer` component in `rust-toolchain.toml`. The tree is brought into compliance by the preparatory stack beneath this branch; this commit records the policy itself.
Bring the tree toward compliance with the hardened `[workspace.lints]` policy, fixing findings at source rather than suppressing them: - apply the machine-applicable clippy fixes across the workspace (`const fn`, owned-string clones, `use_self`, import granularity, format-args interpolation); - restructure panic-prone indexing in the decomposition-advice, LCOM4, clone-detection, and signal modules with iterator and `get`-based rewrites that preserve behaviour; - replace the unsafe `EnvVarGuard`/`LocaleOverride` environment guards with scoped `temp_env`-backed helpers (`with_env_var`, `with_env_var_removed`, `with_locale`) and rework the UI runner and all call sites onto them, satisfying `unsafe_code = "forbid"`; - declare the suite's Dylint entry point through a new `declare_dylint_register_entry!` macro in whitaker-common so the `#[unsafe(no_mangle)]` attribute expands from an external macro; - replace the installer's `libc::geteuid()` root probe with the safe `rustix::process::geteuid()` and drop the now-unused libc dependency; - document struct fields, `# Errors`, and `# Panics` contracts; rename shadowed bindings; add assertion messages; make non-test helpers fallible instead of panicking; - scope the one sanctioned `#[expect(clippy::float_arithmetic)]` to the bumpy-road signal functions whose weights are user-configured floats; - split `lint` into `lint-clippy` and `lint-whitaker` so the Whitaker suite lints the support crates in the gate and CI installs the pinned whitaker-installer; - refresh the spelling exemptions for moved SARIF identifier lines and correct two non-oxendict spellings.
Run `cargo fmt --all` under the new `.rustfmt.toml` (imports granularity, single-line functions, wrapped comments, formatted strings and doc examples). No behavioural changes.
Rename the `-ise`/`-isation` identifiers, call sites, documentation and Gherkin step text to the Oxford `-ize`/`-ization` forms the shared spelling policy requires, rather than exempting each line: - `normalise_*` -> `normalize_*`, `summarise_context*` -> `summarize_context*`, `localised_messages` -> `localized_messages`, and the matching test, fixture and step-function names; - `capitalise_segment`, `pluralisation`, `deserialises_*`, `recognises_*`, `finalised`, `parametrised` and `synthesised` likewise; - SARIF `Artifact`/`ArtifactLocation` -> `Artefact`/`ArtefactLocation`. The `artifactLocation` and `artifacts` property names are fixed by the OASIS SARIF 2.1.0 schema, so the fields pin their wire names with `serde(rename)` and the two literals remain exempt as serialized values; - the en-GB diagnostic note in `no_expect_outside_tests` now reads "recognized", with the byte-exact `.stderr` fixtures updated in step so the localized message and its fallback agree. Delete the unused deprecated `rasterise_signal` alias instead of renaming it; the workspace treats these as internal APIs, so no compatibility shim is needed. Prune the 205 now-dead `typos.local.toml` ignore patterns left behind. The 50 that remain cover genuinely external names only: Gaelic and Welsh locale prose, `cargo_metadata::CompilerArtifact`, GitHub Actions artifact actions, `act`'s `--artifact-server-path`, dylint UI fixture sources, and the two SARIF wire-format properties. Also repair the `#[path]` test declarations in `module_must_have_inner_docs`, which resolved relative to `src/driver/` after that module moved to `driver/mod.rs`, and adopt the shared `allow_fixture_expansion_lints` attribute for single-line rstest fixtures across the workspace.
Satisfy `self_named_module_files` for `install_flow`, `tests`, `toolchain`, and `toolchain::tests::failure_mocks`. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Rename shadowed bindings, borrow needlessly-owned parameters, replace slice indexing with slice patterns, drop the vestigial `Result` around prebuilt installation, and extract wrapper-script assertion helpers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Replace slice indexing with fallible lookups and slice patterns, rename shadowed bindings, name wildcard match variants explicitly, elide a redundant lifetime, inline format arguments, compare archive extensions case-insensitively, and reshape the resolve-crates test case struct to drop its bool cluster. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Bring the lint crates, the root crate, and the installer's directory abstraction into compliance with the hardened policy: - isolate `dylint_linting::impl_late_lint!` in a `mod declaration` per lint crate and re-export only the documented lint surface, so the macro's undocumentable ABI entry point and `get_lints` accessor carry one fulfilled, reasoned `#![expect(missing_docs)]` instead of the policy being disabled crate-wide; - isolate `newt_hype::base_newtype!` likewise for the explicit `Clone` impl it emits on a `Copy` type; - rename the `BaseDirs` trait methods to `home`/`executables`/ `whitaker_data` so the mockall-generated struct no longer shares a uniform field postfix, fixing `struct_field_names` at source; - replace the two `float_cmp` assertions with `to_bits()` comparisons, which is what "the configured value round-tripped unchanged" actually asserts; - add the missing assertion messages in the root crate's harness tests. Scope the Clippy gate to `--lib --bins --tests --benches` rather than `--all-targets`. Every file under a lint crate's `examples/` directory is a `fail_*`/`pass_*` Dylint UI fixture that deliberately contains the anti-patterns the suite detects; a fixture proving `expect_used` fires cannot itself forbid `expect_used`. They live in `examples/` rather than `ui/` because `dylint_testing::Test::example` builds them with each crate's dev-dependencies. `typecheck` still compiles them and the UI harness still lints them.
Three test failures traced to the new formatting profile rather than to any behavioural change: - `cargo fmt --all` formats `examples/` because those files are Cargo targets, and `format_strings` split a long `reason = "..."` across lines. That shifted the diagnostics one line down, so the byte-exact `.stderr` expectations no longer matched. Fixtures under `ui/` were never affected because they are not targets. Add a `.rustfmt.toml` `ignore` list so the `examples/` fixtures are equally immune, and restore the reformatted files; they now differ from `main` only by the two deliberate spelling corrections. - `cargo package -p whitaker-common` failed because the new `whitaker_test_macros` dev-dependency carried a version requirement while the crate is `publish = false`. Make the workspace entry path-only: Cargo strips versionless dev-dependencies when packaging. Also clear the two findings the Whitaker suite raised against its own sources, now that `make lint` runs it: - move the `#[must_use]` attributes in `i18n/diagnostics.rs` back after their doc comments, where a mechanical `clippy --fix` pass had wedged them between the summary line and the rest of the comment (`function_attrs_follow_docs`); - recover the poisoned lock in `env_test_guard` with `PoisonError::into_inner` instead of panicking (`no_unwrap_or_else_panic`). The mutex guards `()`, so a poisoned lock carries no corrupted state -- only the record that an earlier test panicked while holding the serialization token.
`format_strings = true` broke a long literal across lines *inside* an escape sequence: the backslash of `\n` became the end-of-line continuation, leaving the `n` as literal text on the next line. The assertion in the example-harness UI test was then searching for `fixture-localnfingerprint=...` and failing. This is content corruption rather than reformatting, and it is silent -- the code still compiles and only a test comparing exact output caught it. Rewrite the literal with `concat!()`, which `AGENTS.md` already prescribes over backslash-escaped newlines and which the formatter cannot split mid-escape.
`make lint` now runs the Whitaker suite over the workspace's own support crates, and this clears what it found there: - `module_must_have_inner_docs`: give every `mod tests`/`mod unix` an explanatory `//!` opening line; - `no_expect_outside_tests`: fixtures and helpers are not tests, so make them fallible and let the `#[test]`/`#[rstest]` body do the unwrapping. Two helpers became macros instead -- `source_span!` because it is used inside `#[case(...)]` where a `Result` cannot be propagated, and `utf8_path!` so the fallible conversion inlines into each test body; - `no_unwrap_or_else_panic`: propagate errors rather than panicking in the closure, converting the enclosing rstest-bdd steps to return `Result<(), String>`; - `no_std_fs_operations`: add the `i18n_packaging` integration-test crate to `dylint.toml`. That test shells out to `cargo package` and walks `$CARGO_TARGET_DIR`, so it is ambient by construction, and in-source suppression provably does not work for this lint. Note that integration-test targets compile as their own crate named after the file, so the existing `whitaker_common` entry did not cover it; - `module_max_lines`: extract `workspace.rs`'s tests to `workspace_tests.rs` (440 lines to 209 + 231); - `bumpy_road_function`: extract `attempt_repository_install`, then group its six parameters into a documented `RepositoryInstallRequest` so the extraction does not simply trade one denied lint for `too_many_arguments`. Also rename two `let entry = entry?` rebinds and hoist an import above the statements that the fixture conversions introduced ahead of it.
`make lint` now passes end to end: rustdoc, Clippy, and the Whitaker suite over the workspace's own support crates. - `no_expect_outside_tests`: convert the installer's fixtures and test helpers to return `Result`, with the `#[test]`/`#[rstest]` body doing the unwrapping. rstest fixtures are consumed as `#[from(fixture)] fixture_res: Result<..>` so the conversion does not trade the lint for `shadow_reuse` on a `let x = x?` rebind. - `no_unwrap_or_else_panic`: propagate rather than panic in the closure. - `module_max_lines`: split `list`, `downloader`, `workspace`, `dependency_binary_helpers` and `deps::tests` into sibling modules, each keeping an explanatory `//!` opening line. - `bumpy_road_function`: extract the complexity clusters, grouping parameters into named structs where an extraction would otherwise exceed the four-argument budget. - `no_std_fs_operations`: add the integration-test crates that stage fixtures ambiently to `dylint.toml`. In-source suppression does not work for this lint, and these targets compile as their own crate named after the file, so they are not covered by the package-level entries.
`make test` now runs the workspace doctests, which surfaced two things. Doctests compile as their own crate and do not inherit the lib's `#![cfg_attr(feature = "dylint-driver", feature(rustc_private))]`, so under `--all-features` every Dylint driver crate fails to link `rustc_driver`. Their examples are exercised by the unit and UI suites instead, so exclude them, mirroring the existing `nextest` exclusions. Also repair two doc examples in `installer/src/dirs.rs` left stale by the `BaseDirs` method rename. `leta rename` updates code references but not doc comments, and `cargo doc` only validates intra-doc links, so nothing caught these until the examples were actually compiled -- which is the point of adding the gate.
`then_node_has_no_neighbours` and `then_neighbours_of_node_are_sorted` each repeated the same `neighbours_of` lookup and out-of-bounds conversion before their differing assertion, which CodeScene flagged as duplication. Add `with_neighbours_of_node` alongside `with_report` and layer it on top, so the successful-build handling stays in one place and each step is left with only its own assertion. Behaviour is unchanged: the step strings, the bounds-error text, the assertion messages, and the `is_sorted_by_key` check are all preserved verbatim.
CodeScene flagged four runs of adjacent bare assertions in the CLI default tests as large assertion blocks. Each was also silent about which field had drifted: a failure reported only the line number. Add `assert_default_conditions`, which takes `(name, condition)` pairs and names the offending field in the panic message, and express each group as a table of named pure boolean queries. The checks themselves are unchanged, including the `verbosity == 0` comparison, which becomes a boolean rather than an `assert_eq!` so it fits the same table. No `PartialEq`/`Eq` derives were added and the production CLI types are untouched; the tests still read each field individually.
`then_pkg_url_present`, `then_bin_dir_present` and `then_default_pkg_fmt` each repeated the same fetch-key-and-compare shape, which CodeScene flagged as duplication. Add `assert_binstall_value_equals` beside `table_str` and reduce each step to a single call. The helper reports a mismatch by returning `Err` rather than asserting. These steps return `Result`, so an `assert_eq!` here trips `clippy::panic_in_result_fn`, and returning the message matches how every other step in the file reports failure. The message text is unchanged, so existing failures read exactly as before. `then_windows_override_pkg_fmt` is left alone: it reads through the `overrides` table and carries its own error strings, so it does not fit this helper.
Extract shared output assertions so related BDD steps retain their existing conditions while avoiding duplicated control flow.
|
Warning Your free Security trial is over. An organization admin can activate billing to continue. |
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Update: D-9 accepted as option (b); R-1 discharged by spikePlan status is now ADR 002 amendments
R-1 discharged:
|
Draft the execution plan for roadmap item 1.3.1: the `crates/whitaker_support_macros` proc-macro crate implementing `#[dylint_expect(lib = ..., lints(...), reason = ...)]` per ADR 002. The plan decomposes the work so the one genuinely general property -- argument-key well-formedness and order-independence -- lands on a pure function over a three-symbol alphabet, provable in Verus and exhaustively enumerable in a test, while token-tree behaviour is covered by parameterized, snapshot, property, and trybuild fixtures. Scope was extended by user direction to include crates.io publish wiring, the ADR 002 status change to Accepted, and a Dylint suite design cross-reference. An opening prototyping milestone answers the empirical questions the design rests on -- notably whether `#[expect(...)]` works at all for Dylint-registered lints -- before any production code is written.
Run the prototyping milestone during planning rather than deferring it, and record what it found. ADR 002's mandated expansion does not suppress `unexpected_cfgs`: that diagnostic resolves during cfg-expansion, before the annotated item's own lint levels are in scope, so a sibling `#[allow]` arrives too late. Verified end-to-end with a real proc macro emitting the ADR's exact four attributes. Two of the remaining three mandated attributes suppress diagnostics the gated form never emits, and the third removes the only signal that catches a misspelt lint name. Record the deviation as D-9 with three options and a recommendation, and set the plan to BLOCKED pending explicit acceptance. Fold in a six-lens design review. Cut the Verus sidecar, the permutation property, the BDD feature file, and two new assertion crates: the argument-key alphabet has three symbols, so by pigeonhole the 121-case enumeration is a total decision procedure and the proof would have restated a decidable property. Flatten the module layout after a misread of `self_named_module_files`; rename `grammar` to `keys` because it never covered arity; carry position in `ArgShapeError` so diagnostics can be anchored; hold `LitStr` in the newtypes so spans survive and the library name gains a real invariant; drop `syn`'s `full` feature before it leaks to downstream consumers; publish the new crate last and guard the block; correct the non-existent `whitaker_lints` library name throughout; replace INV-DIAG-1's non-vacuity control, which was itself vacuous.
Amend ADR 002 per the accepted deviation D-9(b) and move it to Accepted. The macro expands to the `cfg_attr` gate alone; the `check-cfg` manifest entry is documented as the mechanism rather than a convenience, because a sibling `#[allow(unexpected_cfgs)]` provably cannot suppress a diagnostic resolved during cfg-expansion. Correct Option D's rejection rationale, which rested on two false premises: the gated form emits neither `unknown_lints` nor `clippy::allow_attributes`. Correct two rows of Table 1 accordingly -- "works in downstream crates without extra config" was the decisive argument for Option C over Option D and is in fact Low for every option. Replace the non-existent `whitaker_lints` library name with `whitaker_suite`. Discharge R-1 with a spike, since discarded. `#[expect(...)]` does work for Dylint-registered Whitaker lints: against an individual lint library built from current source, item-level allow, item-level expect, module-level allow and the gated expect all suppress correctly with no spurious unfulfilled-expectation warnings. Record R-1b, which the spike surfaced. The aggregated `whitaker_suite` library ignores lint-level attributes entirely and emits a spurious `unfulfilled_lint_expectations` warning for every expect, while individual libraries built from the same commit behave correctly. Staleness, lint-identity mismatch and cfg_attr interaction were each ruled out by controlled comparison. Since the suite is what every installed consumer loads, no attribute-based suppression works in practice today. This gates ADR 002 migration phase 3, not 1.3.1 delivery, whose obligations are all token-level. Move the plan to APPROVED.
fcbe353 to
114e6bf
Compare
9b758d1 to
afb1e4a
Compare
Summary
ExecPlan for roadmap item 1.3.1 — the
crates/whitaker_support_macrosproc-macro crate implementing#[dylint_expect(lib, lints, reason)]per ADR 002.📄 Plan:
docs/execplans/1-3-1-add-whitaker-support-macros-proc-macro-crate.mdStatus:
BLOCKED. The plan's prototyping milestone was run during planning rather than deferred, and it falsified a premise of ADR 002. A proposed architecture deviation (D-9) is recorded with three options and a recommendation; no implementation may begin until one is accepted.What the prototype found
ADR 002's mandated four-attribute expansion does not suppress
unexpected_cfgs. That diagnostic is resolved during cfg-expansion, before the annotated item's own lint levels are in scope, so a sibling#[allow(unexpected_cfgs)]arrives too late. An attribute macro cannot place theallowin an enclosing scope without wrapping the item and changing its semantics.Verified end-to-end with a real
proc-macro = truecrate emitting the ADR's exact attributes, consumed by a crate carrying this workspace's lint policy — it still warns at the call-site.cfg_attrgate, noallowsunexpected_cfgscheck-cfg = ['cfg(dylint_lib, values(any()))']allow(unexpected_cfgs)on enclosing module / as inner attributeunknown_lintsfires — the typo safety netTwo of the remaining three mandated attributes suppress diagnostics the gated form never emits. The third,
allow(unknown_lints), removes the only signal that catches a misspelt lint name — turning every typo into a silent no-op. ADR 002 §Options considered rejects Option D partly on these grounds, so that rationale needs correcting too.Decision required (D-9)
cfg_attrgate alone, Whitaker adds onecheck-cfgentry and documents it as the consumer prerequisite. Preserves typo detection.dylint_expect_shapelint that validates call-sites against loaded libraries and registered lint names — closing all three silent-no-op routes no macro can. Removes 1.3.1–1.3.4 as written.The plan is written for (b) and ready to execute on acceptance.
Design review
A six-lens expert panel reviewed the first draft. Resulting changes:
googletest/pretty_assertions. The argument-key alphabet has three symbols, so by pigeonhole the 121-case enumeration is a total decision procedure — the proof would have restated a decidable property, whichAGENTS.mdforbids.make verusalso runs in no CI workflow, and no proof file inverus/has ever been modified.clippy::self_named_module_files; renamedgrammartokeys, which is all it ever validated.ArgShapeErrorto carry position so diagnostics can be anchored; newtypes now holdLitStrso spans survive andLibraryNamegains a real invariant.syn'sfullfeature before it leaks into the published manifest and onto every downstream build graph.whitaker-installerin an all-or-nothingset -euxo pipefailblock.whitaker_lintsthroughout — no such library exists; the real names arewhitaker_suiteand the per-lint crates..stderrfiles always differ).Follow-up work identified
dylint_expect_shapelint (proposed 1.3.5) — the only mechanism that can close the wrong-liband misspelt-lint silent no-ops.crates/whitaker_test_macrosemits#[cfg_attr(clippy, expect(clippy::allow_attributes, ...))], which the current toolchain rejects withclippy::unnecessary_clippy_cfg.Verification
make markdownlintandmake nixiepass. No code changes in this PR.References
🤖 Generated with Claude Code
Summary by Sourcery
Approve and document the revised plan for adding conditional Dylint expectation macros after validating the original expansion and identifying remaining integration risks.
Enhancements:
whitaker_support_macrosproc-macro, reducing the expansion to a cfg-gated Dylint expectation and requiring a consumercheck-cfgentry.Documentation: